-
Notifications
You must be signed in to change notification settings - Fork 395
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(wire-service): rfc implementation #166
Conversation
- Add todo-api with getTodo (imperative) and wire adapter - Update single-wire demo to use new getTodo - Update build steps to work with typescript - Kill old code Outstanding: - analyze def wires to know which adapters depend on which vars - ctr the adapters, setup their wireUpdated callback
- More structure and types
return { | ||
updatedCallback: (newConfig) => { | ||
config = newConfig; | ||
subscription = getObservable(config).subscribe({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any time this code gets a new subscription, it should call subscription.unsubscribe()
if subscription
is defined and non-null. Wire adapter implementors may mess that up when dealing with raw Observables, so it's important to highlight that.
buildContext | ||
} from './wiring'; | ||
export interface WiredValue { | ||
data?: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we be more specific here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WiredValue: any
removeEventListener(type: string, listener: WireEventTargetListener): void; | ||
} | ||
|
||
export type WireAdapterFactory = (eventTarget: WireEventTarget) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably rename this to WireAdapterCallback since it is not a factory anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not really a callback either, it's more like a setup... which factory kinda make sense, but i understand it's not returning a lifecycle hook object so it's not a factory.
if (adapterFactory) { | ||
const wireEventTarget = new WireEventTarget(cmp, def, context, wireDef, wireTarget); | ||
adapterFactory({ | ||
dispatchEvent: wireEventTarget.dispatchEvent.bind(wireEventTarget), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why is all these important. Eventually we can iron this out! not a big deal.
* @param paramValues values for all wire adapter config params | ||
*/ | ||
function invokeConfigListeners(configListenerMetadatas: Set<ConfigListenerMetadata>, paramValues: any) { | ||
for (const metadata of configListenerMetadatas) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avoid using iterators. let's use a regular for loop
|
||
function updated(cmp: Element, prop: string, configContext: ConfigContext) { | ||
if (!configContext.mutated) { | ||
configContext.mutated = new Set<string>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really need a Set? can a regular array make the cut so we can just use for-loop on it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
has to be a set to dedupe
if (!configContext.mutated) { | ||
configContext.mutated = new Set<string>(); | ||
// collect all prop changes via a microtask | ||
Promise.resolve().then(updatedFuture.bind(undefined, cmp, configContext)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe an arrow here is better than an actual bind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aside from the small issue with for-of, I think we are good! Also, lets make sure that we do a quick pass on the ES5 version of the transpiled code so feel comfortable with it.
Benchmark comparisonBase commit:
|
Benchmark comparisonBase commit:
|
Details
implements #148
Does this PR introduce a breaking change?